Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: Use 32-bit vector_math #3405

Closed
wants to merge 9 commits into from
Closed

feat!: Use 32-bit vector_math #3405

wants to merge 9 commits into from

Conversation

renancaraujo
Copy link
Member

@renancaraujo renancaraujo commented Dec 14, 2024

Description

Change to 32-bit vector_math to not have to not have incompatible types with 3D and shader code.

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Related Issues

#3406

@renancaraujo renancaraujo changed the title feat: use vector_math 32 feat!: use vector_math 32 Dec 14, 2024
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@renancaraujo renancaraujo changed the title feat!: use vector_math 32 feat!: Use vector_math 32 Dec 14, 2024
@renancaraujo renancaraujo marked this pull request as ready for review December 14, 2024 21:06
spydon pushed a commit to flame-engine/forge2d that referenced this pull request Dec 14, 2024
# Description

Related to flame-engine/flame#3405

## Checklist

<!-- Before you create this PR confirm that it meets all requirements
listed below by checking the
relevant checkboxes (`[x]`). This will ensure a smooth and quick review
process. -->

- [x] The title of my PR starts with a [Conventional Commit] prefix
(`fix:`, `feat:`, `docs:` etc).
- [x] I have read the [Contributor Guide] and followed the process
outlined for submitting PRs.
- [x] I have updated/added tests for ALL new/updated/fixed
functionality.
- [x] I have updated/added relevant documentation in `docs` and added
dartdoc comments with `///`.
- [x] I have updated/added relevant examples in `examples`.

## Breaking Change

<!-- Does your PR require Flame users to manually update their apps to
accommodate your change?

If the PR is a breaking change this should be indicated with suffix "!"
(for example, `feat!:`, `fix!:`). See [Conventional Commit] for details.
-->

- [x] Yes, this is a breaking change.
- [ ] No, this is *not* a breaking change.

## Related Issues

<!-- Provide a list of issues related to this PR from the [issue
database].
Indicate which of these issues are resolved or fixed by this PR, i.e.
Fixes #xxxx* !-->

<!-- Links -->
[issue database]: https://github.com/flame-engine/flame/issues
[Contributor Guide]:
https://github.com/flame-engine/flame/blob/main/CONTRIBUTING.md
[Flame Style Guide]:
https://github.com/flame-engine/flame/blob/main/STYLEGUIDE.md
[Conventional Commit]: https://conventionalcommits.org
@renancaraujo renancaraujo requested a review from spydon December 14, 2024 21:18
@renancaraujo renancaraujo force-pushed the renan/vecmath_32 branch 3 times, most recently from b4ef4ba to 3f2d769 Compare December 14, 2024 21:55
@spydon spydon changed the title feat!: Use vector_math 32 feat!: Use 32-bit vector_math Dec 14, 2024
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@renancaraujo renancaraujo force-pushed the renan/vecmath_32 branch 2 times, most recently from abc06cc to 61c83c6 Compare December 14, 2024 22:35
@renancaraujo
Copy link
Member Author

renancaraujo commented Dec 16, 2024

After changing the imports across the board, i think we should not merge this.
Specially because some crucial tests on collision detection reports some false negatives after this change. This lack of precisioon can mount up and create noticible differences in value changes in sequence (see how we changed tollerance epsilons on the sequence effect test).
I know that nobody should care about values after 5/6 digits after the comma speciallly when it comes down to pixels on users screen, but when it comes down to rotation, scaling and collision detection, i feel very unconfortable with the changes i had to do on tests in order to make them pass

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants